Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display tooltip on button hover and focus #839

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented May 19, 2017

Closes #531

This pull request seeks to implement a tooltip to be shown on hover and focus for <Button /> components passed a title prop.

Tooltip

Disabled save

Implementation notes:

This turned out to be quite complex implement, mainly due to the nature of recreating disabled button behaviors manually. This was necessary because mouse events are not fired on disabled elements. The mouseover events are used to handle the case where tooltips exceeds page content bounds, which itself turned out to be complex to implement. Tooltips should flip vertical if exceeding the top of page content, or stretch left or right if exceeding right or left bounds of page content.

I expect that tooltip behavior could be generalized outside the Button case, but decided against preoptimizing for this need in this iteration.

Because of the effort needed to handle focus and mouse events, it's tempting to not bother with leveraging :hover and :focus selectors with :before and :after pseudo-elements, but rather render a custom element. One advantage this could have is that it would avoid the need to "reset" tooltip styles which are otherwise inherited from the button (like font, text shadow, color, etc).

Testing instructions:

Verify that normal tab and focus behaviors exist for disabled buttons: they are skipped by tab presses, do not receive focus, and do not trigger click callbacks.

Verify that both disabled and enabled buttons with titles show tooltips on both hover and on focus events. Notably those in the header and block toolbars.

Verify that tooltips exceeding top, right, or left bounds of page content are reoriented to fit correctly. Notably the disabled "Save" button in the production build (npm run build).

@aduth aduth added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label May 19, 2017
@jasmussen
Copy link
Contributor

Love this.

There are some padding and size tweaks I'd like to do, but nothing that needs to hold this off. Thanks for doing this.

@aduth aduth force-pushed the add/button-label-tooltip branch from d1a964b to f36e1de Compare May 19, 2017 16:41
@aduth aduth mentioned this pull request May 19, 2017
@aduth
Copy link
Member Author

aduth commented May 19, 2017

Rebased to account for changes from #838.

Noticing another issue with leveraging pseudo-elements is that we'll be fighting for access to :before and :after. For example, the heading block controls use pseudo-elements for heading level, which appear quick broken combined with this tooltip behavior:

Heading Level Tooltip

This lending more credibility to my original consideration of "it's tempting to not bother with leveraging :hover and :focus selectors with :before and :after pseudo-elements, but rather render a custom element."

@jasmussen
Copy link
Contributor

See also discussion in #783 (comment) for an alternate approach. As with all of those ideas: low priority, lean towards code simplicity.

@aduth
Copy link
Member Author

aduth commented May 22, 2017

@jasmussen Specifically are you referring to the possibility that we might not need subscripts in the controls if we use a dropdown instead?

@jasmussen
Copy link
Contributor

Specifically are you referring to the possibility that we might not need subscripts in the controls if we use a dropdown instead?

Yes. The only other use case for numbers in the control buttons are for #546, where I think we might want to do something different regardless.

@jasmussen
Copy link
Contributor

Took a look at this branch again. I think it would still be really nice to have a good tooltip component, but due to the complexities with pseudo elements perhaps we should pause this for now. Since this PR there's also been added a label to the Inserter, which was the biggest issue at the time.

Perhaps we should simply go with title attributes for now, with no custom decoration?

@aduth
Copy link
Member Author

aduth commented Jul 31, 2017

Perhaps we should simply go with title attributes for now, with no custom decoration?

I think this is at odds with current direction in core with respect to title attributes:

https://core.trac.wordpress.org/ticket/24766

@jasmussen
Copy link
Contributor

I think this is at odds with current direction in core with respect to title attributes:

I do agree it'd be best with tooltips. Just wondered if we could hold over with just the title attributes for the time being.

@aduth
Copy link
Member Author

aduth commented Aug 8, 2017

Tooltips are much simpler to implement with recent enhancements to the Popover component. I'm going to close this pull request because it's turned unwieldy to rebase, and will pull some of the revisions into a new, separate pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants